Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test accumulating gradient collector #2111

Merged
merged 1 commit into from
Nov 1, 2022

Conversation

zachgk
Copy link
Contributor

@zachgk zachgk commented Oct 31, 2022

This adds additional documentation and testing to define the API for the gradient collector. Specifically, it focuses on the gradient accumulating behavior and adds a restriction to creating multiple simultaneous global gradient collectors.

It also raises an issue for dealing with MXNet to accumulate gradients. I investigated both the setRequiresGradient with ADD mode and backwards with retainGrads and neither was correctly implementing the gradient accumulation behavior.
As this is not a terribly significant use case, I will just leave it as a TODO until there are any users who specifically request it.

This adds additional documentation and testing to define the API for the
gradient collector. Specifically, it focuses on the gradient accumulating
behavior and adds a restriction to creating multiple simultaneous global
gradient collectors.

It also raises an issue for dealing with MXNet to accumulate gradients. I
investigated both the setRequiresGradient with ADD mode and backwards with
retainGrads and neither was correctly implementing the gradient accumulation
behavior.
As this is not a terribly significant use case, I will just leave it as a TODO
until there are any users who specifically request it.
@zachgk zachgk requested a review from frankfliu as a code owner October 31, 2022 23:19
@codecov-commenter
Copy link

Codecov Report

Base: 72.08% // Head: 71.36% // Decreases project coverage by -0.72% ⚠️

Coverage data is based on head (e0499ff) compared to base (bb5073f).
Patch coverage: 71.65% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #2111      +/-   ##
============================================
- Coverage     72.08%   71.36%   -0.73%     
- Complexity     5126     6279    +1153     
============================================
  Files           473      624     +151     
  Lines         21970    27800    +5830     
  Branches       2351     2998     +647     
============================================
+ Hits          15838    19839    +4001     
- Misses         4925     6502    +1577     
- Partials       1207     1459     +252     
Impacted Files Coverage Δ
api/src/main/java/ai/djl/modality/cv/Image.java 69.23% <ø> (-4.11%) ⬇️
...rc/main/java/ai/djl/modality/cv/MultiBoxPrior.java 76.00% <ø> (ø)
...rc/main/java/ai/djl/modality/cv/output/Joints.java 71.42% <ø> (ø)
.../main/java/ai/djl/modality/cv/output/Landmark.java 100.00% <ø> (ø)
...main/java/ai/djl/modality/cv/output/Rectangle.java 72.41% <0.00%> (ø)
...i/djl/modality/cv/translator/BigGANTranslator.java 21.42% <0.00%> (-5.24%) ⬇️
.../modality/cv/translator/ImageFeatureExtractor.java 0.00% <0.00%> (ø)
.../ai/djl/modality/cv/translator/YoloTranslator.java 27.77% <0.00%> (+18.95%) ⬆️
...modality/cv/translator/wrapper/FileTranslator.java 44.44% <ø> (ø)
...y/cv/translator/wrapper/InputStreamTranslator.java 44.44% <ø> (ø)
... and 563 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@zachgk zachgk merged commit 909daba into deepjavalibrary:master Nov 1, 2022
@zachgk zachgk deleted the accumGradCol branch November 1, 2022 18:57
KexinFeng added a commit to KexinFeng/djl that referenced this pull request Jan 7, 2023
KexinFeng added a commit that referenced this pull request Jan 7, 2023
* Revert "Test accumulating gradient collector (#2111)"

This reverts commit 909daba

* Revert "Ensure GradientCollector can clear gradients (#2101)"

This reverts commit 0972264.

* Add NDManager.zeroGradients()

Co-authored-by: Frank Liu <frankfliu2000@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants